Skip to content

[CLEAN] Synthetic Benchmark PR #138650 - Make AggregateMetricDoubleFieldType immutable #23

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
tomerqodo wants to merge 1 commit into
base: base_pr_138650_20251204_8186
Choose a base branch
from
Open

[CLEAN] Synthetic Benchmark PR #138650 - Make AggregateMetricDoubleFieldType immutable #23

tomerqodo wants to merge 1 commit into from

Conversation

@tomerqodo
Copy link

@tomerqodo tomerqodo commented Dec 4, 2025 •
edited by qodo-code-review bot
Loading

User description

Benchmark PR elastic#138650

Type: Clean (correct implementation)

Original PR Title: Make AggregateMetricDoubleFieldType immutable
Original PR Description: unknown
Original PR URL: elastic#138650

PR Type

Enhancement

Description

  • Make AggregateMetricDoubleFieldType immutable by converting mutable fields to final

  • Move field initialization from setter methods to constructor parameters

  • Remove setter methods (setMetricFields, setDefaultMetric, addMetricField)

  • Update all test files to use new immutable constructor signature

  • Remove unused IndexType import from mapper file

Diagram Walkthrough

flowchart LR
  A["Mutable fields<br/>with setters"] -- "Convert to final<br/>fields" --> B["Immutable constructor<br/>initialization"]
  C["Test helper methods<br/>using setters"] -- "Update to use<br/>new constructor" --> D["Direct field<br/>initialization"]
Loading

File Walkthrough
Relevant files
Enhancement
AggregateMetricDoubleFieldMapper.java
Make AggregateMetricDoubleFieldType immutable                       

x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateMetricDoubleFieldMapper.java

  • Convert metricFields and defaultMetric fields from mutable to final
  • Replace constructor overloads with single constructor accepting all
    required parameters
  • Move field initialization from setter methods into constructor
  • Remove setMetricFields(), setDefaultMetric(), and addMetricField()
    setter methods
  • Update builder to pass all parameters to constructor instead of
    calling setters
  • Remove unused IndexType import
 +16/-34 
Tests
AggregateMetricBackedAvgAggregatorTests.java
Update test to use immutable constructor                                 

x-pack/plugin/mapper-aggregate-metric/src/test/java/org/elasticsearch/xpack/aggregatemetric/aggregations/metrics/AggregateMetricBackedAvgAggregatorTests.java

  • Update createDefaultFieldType() to use new immutable constructor
  • Build metricFields map before constructor call instead of using setter
  • Add imports for EnumMap and Map
  • Pass all parameters directly to constructor
 +5/-5     
AggregateMetricBackedMaxAggregatorTests.java
Update test to use immutable constructor                                 

x-pack/plugin/mapper-aggregate-metric/src/test/java/org/elasticsearch/xpack/aggregatemetric/aggregations/metrics/AggregateMetricBackedMaxAggregatorTests.java

  • Update createDefaultFieldType() to use new immutable constructor
  • Build metricFields map before constructor call instead of using setter
  • Add imports for EnumMap and Map
  • Pass all parameters directly to constructor
 +5/-5     
AggregateMetricBackedMinAggregatorTests.java
Update test to use immutable constructor                                 

x-pack/plugin/mapper-aggregate-metric/src/test/java/org/elasticsearch/xpack/aggregatemetric/aggregations/metrics/AggregateMetricBackedMinAggregatorTests.java

  • Update createDefaultFieldType() to use new immutable constructor
  • Build metricFields map before constructor call instead of using setter
  • Add imports for EnumMap and Map
  • Pass all parameters directly to constructor
 +5/-5     
AggregateMetricBackedSumAggregatorTests.java
Update test to use immutable constructor                                 

x-pack/plugin/mapper-aggregate-metric/src/test/java/org/elasticsearch/xpack/aggregatemetric/aggregations/metrics/AggregateMetricBackedSumAggregatorTests.java

  • Update createDefaultFieldType() to use new immutable constructor
  • Build metricFields map before constructor call instead of using setter
  • Add imports for EnumMap and Map
  • Pass all parameters directly to constructor
 +5/-5     
AggregateMetricBackedValueCountAggregatorTests.java
Update test to use immutable constructor                                 

x-pack/plugin/mapper-aggregate-metric/src/test/java/org/elasticsearch/xpack/aggregatemetric/aggregations/metrics/AggregateMetricBackedValueCountAggregatorTests.java

  • Update createDefaultFieldType() to use new immutable constructor
  • Build metricFields map before constructor call instead of using setter
  • Add imports for EnumMap and Map
  • Pass all parameters directly to constructor
 +5/-5     

@qodo-code-review
Copy link

qodo-code-review bot commented Dec 4, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
⚪
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

⚪
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No audit logs: The new constructor-based initialization and field changes add no logging for critical
actions, but given this is a mapper type refactor, it may not require audit trails.

Referred Code 
        EnumMap<Metric, NumberFieldMapper.NumberFieldType> metricFields = metricMappers.entrySet()
            .stream()
            .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().fieldType(), (l, r) -> {
                throw new IllegalArgumentException("Duplicate keys " + l + "and " + r + ".");
            }, () -> new EnumMap<>(Metric.class)));

        AggregateMetricDoubleFieldType metricFieldType = new AggregateMetricDoubleFieldType(
            context.buildFullName(leafName()),
            timeSeriesMetric.getValue(),
            defaultMetric.getValue(),
            metricFields,
            meta.getValue()
        );

        return new AggregateMetricDoubleFieldMapper(leafName(), metricFieldType, metricMappers, builderParams(this, context), this);
    }
}

public static final FieldMapper.TypeParser PARSER = new TypeParser(
    (n, c) -> new Builder(n, c.getIndexSettings()),
    notInMultiFields(CONTENT_TYPE)


 ... (clipped 20 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Null safety risk: The constructor calls metricFields.get(defaultMetric) without visible null/absence checks
in the diff, which could NPE if inputs are invalid, though validations may exist
elsewhere.

Referred Code 
public AggregateMetricDoubleFieldType(
    String name,
    MetricType metricType,
    Metric defaultMetric,
    EnumMap<Metric, NumberFieldMapper.NumberFieldType> metricFields,
    Map<String, String> meta
) {
    super(name, metricFields.get(defaultMetric).indexType(), false, meta);
    this.metricType = metricType;
    this.defaultMetric = defaultMetric;

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Input validation: New constructor relies on provided defaultMetric and metricFields but the diff does not
show validation ensuring defaultMetric exists in metricFields and that metricFields is
non-empty.

Referred Code 
public AggregateMetricDoubleFieldType(
    String name,
    MetricType metricType,
    Metric defaultMetric,
    EnumMap<Metric, NumberFieldMapper.NumberFieldType> metricFields,
    Map<String, String> meta
) {
    super(name, metricFields.get(defaultMetric).indexType(), false, meta);
    this.metricType = metricType;
    this.defaultMetric = defaultMetric;
    this.metricFields = metricFields;
}

Learn more about managing compliance generic rules or creating your own custom rules
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

qodo-code-review bot commented Dec 4, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add null check for default metric

Add a null check for the default metric in the AggregateMetricDoubleFieldType
constructor to prevent a potential NullPointerException and instead throw an
IllegalArgumentException.

x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateMetricDoubleFieldMapper.java [283]

-super(name, metricFields.get(defaultMetric).indexType(), false, meta);
+NumberFieldMapper.NumberFieldType defaultFieldType = metricFields.get(defaultMetric);
+if (defaultFieldType == null) {
+    throw new IllegalArgumentException("Default metric " + defaultMetric + " must be present in metric fields");
+}
+super(name, defaultFieldType.indexType(), false, meta);

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a potential NullPointerException and proposes adding a guard clause to the constructor, which improves robustness by providing a clearer error message.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Review effort 2/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tomerqodo